Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interactive UDC withdraw #2635

Merged
merged 10 commits into from
Mar 25, 2021
Merged

Interactive UDC withdraw #2635

merged 10 commits into from
Mar 25, 2021

Conversation

andrevmatos
Copy link
Contributor

@andrevmatos andrevmatos commented Mar 24, 2021

Fixes #2629

Short description
Refactor: split services/epics into separate modules for UDC handling, MS and PFS.
udcWithdraw is split into 2 sets of independent async actions: udc/withdraw/plan, which is responsible only for the planning transaction, per user request, and triggered from Raiden.planUdcWithdraw public method, and udc/withdraw, which triggers the final step, after 100 blocks of the planned tx, which got exposed through Raiden.udcWithdraw method.
Additionally, the current plan (if any) can be checked from the new Raiden.getUdcWithdrawPlan method, which will resolve to undefined if there's none, or to an object containing amount, block (block at which it'll become ready) and ready (boolean to tell if it's already ready).
A new config.autoUdcWithdraw, which defaults to true, enables an epic which will watch the blockchain and automatically udc/withdraw/request when a plan is ready; it can be turned off, and only then Raiden.udcWithdraw is made available for the interactive 2nd step udcWithdraw.

Definition of Done

  • Steps to manually test the change have been documented
  • Acceptance criteria are met
  • Change has been manually tested by the reviewer (dApp)

Steps to manually test the change (dApp)

@codecov
Copy link

codecov bot commented Mar 24, 2021

Codecov Report

Merging #2635 (9b6f0a6) into master (0a7f39d) will decrease coverage by 0.16%.
The diff coverage is 93.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2635      +/-   ##
==========================================
- Coverage   89.84%   89.68%   -0.17%     
==========================================
  Files         172      175       +3     
  Lines        7004     7089      +85     
  Branches     1177     1185       +8     
==========================================
+ Hits         6293     6358      +65     
- Misses        623      643      +20     
  Partials       88       88              
Flag Coverage Δ
dapp 87.49% <100.00%> (ø)
dapp.unit 87.49% <100.00%> (ø)
sdk 90.70% <93.14%> (-0.26%) ⬇️
sdk.e2e 60.09% <67.54%> (+0.33%) ⬆️
sdk.integration 0.26% <0.00%> (-0.01%) ⬇️
sdk.unit 85.08% <92.33%> (-0.16%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
raiden-ts/src/actions.ts 100.00% <ø> (ø)
raiden-ts/src/channels/utils.ts 97.53% <ø> (ø)
raiden-ts/src/config.ts 94.87% <ø> (ø)
raiden-ts/src/raiden.ts 53.79% <5.00%> (-2.01%) ⬇️
raiden-ts/src/utils/rx.ts 95.78% <85.71%> (-3.07%) ⬇️
raiden-ts/src/services/epics/helpers.ts 94.41% <94.41%> (ø)
raiden-ts/src/services/epics/pathfinding.ts 98.95% <98.95%> (ø)
...app/src/components/dialogs/UdcWithdrawalDialog.vue 97.67% <100.00%> (ø)
raiden-dapp/src/services/raiden-service.ts 83.73% <100.00%> (ø)
raiden-ts/src/constants.ts 100.00% <100.00%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a7f39d...9b6f0a6. Read the comment docs.

@andrevmatos andrevmatos force-pushed the feature/split_udc_withdraw branch from 290ab82 to a4fae97 Compare March 24, 2021 14:26
@github-actions
Copy link

github-actions bot commented Mar 24, 2021

You modified raiden-dapp/src,
Please remember to add a change log entry at raiden-dapp/CHANGELOG.md if necessary.

...into pathfinding, monitor and udc epics, since the generic epic
module was already too big.
Now named udcWithdraw, and the previous udcWithdraw is renamed to
udcWithdrawPlan. Also, a small refactoring to check if totalDeposit is
outdated late, right before calling deposit (after approveIfNeeded$).
@andrevmatos andrevmatos force-pushed the feature/split_udc_withdraw branch from a4fae97 to 4e81cd6 Compare March 24, 2021 18:41
@andrevmatos andrevmatos self-assigned this Mar 24, 2021
@andrevmatos andrevmatos requested a review from weilbith March 24, 2021 18:42
@andrevmatos andrevmatos added enhancement New feature or request sdk 🖥 labels Mar 24, 2021
@andrevmatos andrevmatos marked this pull request as ready for review March 24, 2021 18:42
udcWithdraw phase (after plan) is now a request/success/failure epic.
udcWithdraw.request is handled, dispatches the transaction, wait for it
to be mined and output either success or failure.
If config.autoUdcWithdraw is enabled (default), an epic will watch the
blockchain for plans from us and dispatch the udcWithdraw.request
The former queries the UDC for currently planned withdraws, returning
amount, block in which it becomes available and ready state; the later
is to be called when config.autoUdcWithdraw is false and after plan is
'ready', and interactively performs the last UDC withdraw phase.
@andrevmatos andrevmatos force-pushed the feature/split_udc_withdraw branch from 4e81cd6 to 63280a5 Compare March 24, 2021 22:14
raiden-ts/src/raiden.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@weilbith weilbith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time and implementing this feature. What sound to easy in the beginning actually became quite big.
I'm pretty happy with most of it. Just the epic to auto-trigger withdrawals is not so nice as I think. We should discuss it together. Especially the hard binding to the exact contract deployment constructor value is not so nice. I would like to get rid of it.

raiden-ts/src/services/epics/udc.ts Show resolved Hide resolved
raiden-ts/src/utils/error.ts Show resolved Hide resolved
raiden-ts/src/utils/error.ts Show resolved Hide resolved
raiden-ts/src/services/epics/udc.ts Outdated Show resolved Hide resolved
raiden-ts/src/services/epics/udc.ts Outdated Show resolved Hide resolved
raiden-ts/src/services/epics/udc.ts Show resolved Hide resolved
raiden-ts/src/raiden.ts Outdated Show resolved Hide resolved
raiden-ts/src/raiden.ts Outdated Show resolved Hide resolved
raiden-ts/src/raiden.ts Show resolved Hide resolved
Copy link
Contributor

@weilbith weilbith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the long call! 🙏🏾
Here the awaited approval. Good work.

- Raiden.udcWithdraw -> Raiden.withdrawFromUDC
- Raiden.planUdcWithdraw -> Raiden.planUDCWithdraw
- config.autoUdcWithdraw -> config.autoUDCWithdraw
- 100 -> constants.UDC_WITHDRAW_TIMEOUT
@andrevmatos andrevmatos merged commit 6e06d5a into master Mar 25, 2021
@andrevmatos andrevmatos deleted the feature/split_udc_withdraw branch March 25, 2021 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request sdk 🖥
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SDK - split user deposit withdrawal into plan and actual withdraw actions
3 participants